Skip to content

feat: add CalendarIntervalType support - #4898

Merged
andygrove merged 7 commits into
apache:mainfrom
peterxcli:feat/calendar-interval-type
Jul 25, 2026
Merged

feat: add CalendarIntervalType support#4898
andygrove merged 7 commits into
apache:mainfrom
peterxcli:feat/calendar-interval-type

Conversation

@peterxcli

@peterxcli peterxcli commented Jul 12, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

Part of #4540.

Rationale for this change

Comet does not currently support Spark's CalendarIntervalType, causing queries carrying calendar interval columns through Comet operators to fall back to Spark.

This change adds the foundational Arrow and codegen support needed to preserve calendar intervals—months, days, and microseconds—across the Spark, Arrow, and native execution boundaries.

What changes are included in this PR?

  • Map Spark CalendarIntervalType to Arrow Interval(MonthDayNano).
  • Add CALENDAR_INTERVAL to the serialized data type protocol and native Arrow type conversion.
  • Support reading and writing calendar intervals through Arrow vectors, including microsecond-to-nanosecond conversion.
  • Add CalendarIntervalType support to Comet batch-kernel code generation for scalar and nested values.
  • Allow calendar interval columns through Comet’s data type support checks and query plan serialization.
  • Add SQL and unit-test coverage for nullable and non-null calendar interval values.

How are these changes tested?

Added coverage for:

  • Round-tripping CalendarIntervalType through the Arrow writer and CometPlainVector.
  • Round-tripping calendar intervals through the batch-kernel codegen path.
  • SQL queries containing positive, negative, mixed-component, and null calendar interval values with native local table scans enabled.

@peterxcli peterxcli changed the title Add CalendarIntervalType support feat: add CalendarIntervalType support Jul 12, 2026
@andygrove

andygrove commented Jul 17, 2026

Copy link
Copy Markdown
Member

Nice, clean approach to interval support. I traced the micros-to-nanos conversion through all four paths (both writers, both readers) and they are consistent, and the child-vector getInterval chain and slicing both look correct. Limiting the scope to pass-through with no native arithmetic feels right.

A few questions, none blocking:

  • Does an interval column survive a native Comet shuffle? The tests cover localTableScan and codegen round-trips but not an exchange, and this PR makes the type eligible for shuffle. A shuffle test, or an explicit note that shuffle is out of scope for this PR, would help.
  • The codegen adds struct-field and array-element getInterval cases, but the tests are all scalar. Could you add a nested (struct or array) interval test to cover those branches?
  • The codegen output uses Math.multiplyExact(micros, 1000L), which throws on overflow, while the ArrowWriter path relies on Spark's writer, which multiplies without the exact check. For an interval whose microseconds exceed what Arrow's int64 nanoseconds can hold, one path throws and the other wraps. Worth a short comment on the intended behavior for that (rare) case.

Also heads up: the red rust-test check is a runner disk-space flake (No space left on device during linking), not a real failure, so it just needs a re-run.

This review was prepared with the assistance of an LLM (Claude).

peterxcli
peterxcli commented Jul 20, 2026
View reviewed changes
Comment thread spark/src/main/scala/org/apache/comet/udf/codegen/CometScalaUDFCodegen.scala
_: Float4Vector | _: Float8Vector | _: DecimalVector | _: VarCharVector |
_: VarBinaryVector | _: DateDayVector | _: TimeStampMicroVector |
_: TimeStampMicroTZVector =>
_: TimeStampMicroTZVector | _: IntervalMonthDayNanoVector =>

@peterxcli peterxcli Jul 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self reminder: move this to CometLiteral.getSupportLevel after #4715 is merged.

@peterxcli

peterxcli commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

The codegen output uses Math.multiplyExact(micros, 1000L), which throws on overflow, while the ArrowWriter path relies on Spark's writer, which multiplies without the exact check. For an interval whose microseconds exceed what Arrow's int64 nanoseconds can hold, one path throws and the other wraps. Worth a short comment on the intended behavior for that (rare) case.

Seems like ArrowWriter use Math.multiplyExact, too. I think, so they're actually consistent on the overflow exception.

datafusion-comet/spark/src/main/scala/org/apache/spark/sql/comet/execution/arrow/ArrowWriters.scala

Lines 497 to 507 in 155e732

private[arrow] class IntervalMonthDayNanoWriter(val valueVector: IntervalMonthDayNanoVector)
extends ArrowFieldWriter {
override def setNull(): Unit = {
valueVector.setNull(count)
}
override def setValue(input: SpecializedGetters, ordinal: Int): Unit = {
val ci = input.getInterval(ordinal)
valueVector.setSafe(count, ci.months, ci.days, Math.multiplyExact(ci.microseconds, 1000L))
}
}

andygrove
andygrove approved these changes Jul 24, 2026
View reviewed changes

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @peterxcli. Could you fix conflicts?

@peterxcli
peterxcli requested a review from andygrove July 24, 2026 18:21
@andygrove andygrove mentioned this pull request Jul 24, 2026
Open
@andygrove
andygrove merged commit 69df0f1 into apache:main Jul 25, 2026
137 of 138 checks passed
@peterxcli
peterxcli deleted the feat/calendar-interval-type branch July 25, 2026 14:21
@peterxcli

peterxcli commented Jul 25, 2026

Copy link
Copy Markdown
Member Author

@andygrove Thanks for the review!

@mbutrovich mbutrovich mentioned this pull request Jul 27, 2026
Open
andygrove added a commit to andygrove/datafusion-comet that referenced this pull request Jul 28, 2026
@andygrove
feat: support make_interval via codegen dispatch
1ee0d6d 
CalendarIntervalType support landed in apache#4898, so MakeInterval can now go
through the JVM codegen dispatcher alongside TimestampAdd and TimestampDiff
instead of falling the projection back to Spark.

Also broadens the datetime test coverage:

- timestampadd gains MILLISECOND and DAYOFYEAR, a TIMESTAMP_NTZ column
  (a separately compiled kernel, since TimestampAdd.dataType follows the
  input), DST spring-forward, fall-back and nonexistent-local-hour cases,
  and a DATETIME_OVERFLOW expect_error case.
- timestampdiff gains MICROSECOND, MILLISECOND and QUARTER, NTZ inputs,
  and the matching DST cases.
- The grammar routes DATEADD/DATE_ADD and DATEDIFF/DATE_DIFF/TIMEDIFF to
  TimestampAdd/TimestampDiff when the first argument is a datetimeUnit
  keyword. Those alias spellings are now covered, split by the version
  that introduced them: dateadd/datediff inline, date_add/date_diff in
  date_add_unit_alias.sql (Spark 3.5+), timediff in timediff.sql (4.0+).
- make_interval and make_interval_ansi fixtures, the latter covering the
  ANSI overflow path where MakeInterval.failOnError is true.

Corrects the date_add, date_diff, dateadd and datediff rows in the
expression guide, which claimed Native without noting that this is true
only of the two-argument form, and adds rows for the unit spellings.
This was referenced Jul 28, 2026
Open
Open
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andygrove andygrove Awaiting requested review from andygrove

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@peterxcli @andygrove